Ros2 change wheels mount point #492
Conversation
WalkthroughThe changes update several configuration and URDF/XACRO files for the robotic vehicle. In the YAML configuration files, the Changes
Sequence Diagram(s)sequenceDiagram
participant URDF as Robot URDF
participant WheelMacro as Wheel Macro
participant FixedJoint as Fixed Joint
participant BaseLink as Wheel Base Link
participant WheelLink as Wheel Link
URDF->>WheelMacro: Call wheel macro with configuration parameters
WheelMacro->>WheelMacro: Compute mount points using wheelbase/2
WheelMacro->>FixedJoint: Create fixed joint ("base_to_${prefix}_wheel_base_joint")
FixedJoint->>BaseLink: Instantiate wheel base link
BaseLink->>WheelLink: Connect via continuous joint to wheel link
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
husarion_ugv_description/urdf/lynx/lynx_macro.urdf.xacro (1)
17-18: LGTM! Consider adding documentation for the mount point parameters.The renaming to
wheelbaseimproves clarity, and the newwheel_mount_point_yparameter aligns with the PR objective to change wheel mount points.Consider adding XML comments to document:
- The units (meters)
- What these parameters represent in the robot's physical structure
- Any constraints or valid ranges for these values
husarion_ugv_description/urdf/lynx/base.urdf.xacro (1)
22-28: LGTM! Consider documenting inertial properties source.The updated inertial properties appear physically valid:
- The inertia matrix is symmetric
- The values satisfy the parallel axis theorem
- The mass change is minimal and reasonable
Consider adding a comment documenting the source of these inertial properties (e.g., CAD model, physical measurements, etc.).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
husarion_ugv_description/meshes/WH05/fl_wheel.daeis excluded by!**/*.daehusarion_ugv_description/meshes/WH05/fr_wheel.daeis excluded by!**/*.daehusarion_ugv_description/meshes/WH05/rl_wheel.daeis excluded by!**/*.daehusarion_ugv_description/meshes/WH05/rr_wheel.daeis excluded by!**/*.daehusarion_ugv_description/meshes/lynx/base.daeis excluded by!**/*.daehusarion_ugv_description/meshes/lynx/base_collision.daeis excluded by!**/*.daehusarion_ugv_description/meshes/lynx/battery.daeis excluded by!**/*.daehusarion_ugv_description/meshes/lynx/user_compartment.daeis excluded by!**/*.dae
📒 Files selected for processing (8)
husarion_ugv_description/config/WH01.yaml(1 hunks)husarion_ugv_description/config/WH02.yaml(1 hunks)husarion_ugv_description/config/WH04.yaml(1 hunks)husarion_ugv_description/config/WH05.yaml(1 hunks)husarion_ugv_description/urdf/common/wheel.urdf.xacro(1 hunks)husarion_ugv_description/urdf/lynx/base.urdf.xacro(2 hunks)husarion_ugv_description/urdf/lynx/lynx_macro.urdf.xacro(2 hunks)husarion_ugv_description/urdf/panther/panther_macro.urdf.xacro(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (14)
husarion_ugv_description/config/WH04.yaml (1)
3-3: Add New Mount Point Offset ParameterThe
mount_point_offset: 0.06parameter has been added, replacing the oldwheel_separationsetting. This change aligns with the updated wheel mounting configuration. Ensure that any URDF or simulation references to this parameter are updated accordingly.husarion_ugv_description/config/WH01.yaml (1)
3-3: Introduce Mount Point Offset for WH01The new
mount_point_offset: 0.091parameter replaces the previously usedwheel_separationvalue. Please confirm that the value is consistent with the physical and simulation requirements for the WH01 configuration and that all downstream configurations (e.g., URDF/XACRO files) reference the correct parameter.husarion_ugv_description/config/WH02.yaml (1)
3-3: Update Parameter to Use Mount Point Offset in WH02The parameter
mount_point_offset: 0.08165is now used in lieu ofwheel_separation. This update is consistent with the new mounting strategy. Make sure that corresponding models and scripts correctly interpret this new parameter value.husarion_ugv_description/config/WH05.yaml (2)
3-3: Set Mount Point Offset for WH05A new parameter,
mount_point_offset: 0.06, has been added. Verify that this value precisely reflects the intended mounting position for the WH05 configuration compared to other models in the suite.
4-5: Modify Physical Parameters for WH05The
masshas been updated to2.036and theinertiavalues have been slightly adjusted to{ ixx: 0.014738, iyy: 0.02593, izz: 0.014738 }. These changes suggest a design revision for the physical properties. Ensure that these new values are validated against simulation results and the real-world model expectations.husarion_ugv_description/urdf/panther/panther_macro.urdf.xacro (2)
22-23: LGTM! Improved property naming for wheel mounting points.The change from
wheel_separation_xtowheelbaseuses more standard robotics terminology, and the newwheel_mount_point_yproperty clearly indicates its purpose for mounting point coordinates.
37-59: LGTM! Symmetric wheel mounting configuration.The wheel declarations now use explicit mount points with symmetric positioning (wheelbase/2.0), which aligns with the PR objective to mount wheels at designated points.
Let's verify that the wheel.urdf.xacro file has been updated to support these new parameters:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if wheel.urdf.xacro supports the new mount point parameters # Look for wheel_mount_point parameters in wheel.urdf.xacro rg -A 5 "wheel_mount_point" "$(find . -name wheel.urdf.xacro)"Length of output: 1914
Solid wheel configuration!
The output confirms that thewheel.urdf.xacrofile has been updated to support the new mount point parameters. The parameter list now includeswheel_mount_point_xandwheel_mount_point_y, and each conditional block for the wheel prefixes (fl,fr,rl,rr) correctly uses these parameters—applying appropriate sign adjustments to ensure symmetric wheel placement.
- The parameter declaration in
wheel.urdf.xacroincludes:
params="wheel_config prefix wheel_mount_point_x wheel_mount_point_y"- The individual
<xacro:if>blocks correctly reference these parameters to set the wheel positions based on the wheel prefix.This confirms that the changes in
panther_macro.urdf.xacroare fully supported downstream.husarion_ugv_description/urdf/common/wheel.urdf.xacro (3)
5-6: LGTM! Parameter changes align with PR objectives.The updated parameters (
wheel_mount_point_xandwheel_mount_point_y) provide more explicit control over wheel positioning compared to the previouswheel_separation_xparameter, improving clarity of the wheel mounting configuration.
8-35: LGTM! Property definitions maintain wheel symmetry.The property definitions correctly handle:
- Wheel positioning using mount points
- Symmetrical offsets for left/right wheels
- Consistent pattern across all four wheels
37-53: Verify impact on existing configurations and controllers.The new joint structure improves modularity by separating the mounting point (fixed joint) from wheel rotation (continuous joint). This is a good architectural change.
Please verify:
- Existing robot configurations are updated to handle the new joint structure
- Controllers are configured to use the correct joint names
- Simulation models work correctly with the new joint hierarchy
husarion_ugv_description/urdf/lynx/lynx_macro.urdf.xacro (1)
32-54: LGTM! Verify wheel mount point calculations.The changes consistently apply the new mount point parameters across all wheels, with correct symmetrical placement using
wheelbase/2.0.Please verify:
- The wheel mount points match the physical robot's dimensions
- The mount points are compatible with the wheel configuration in the YAML file
- The simulation behaves correctly with these mount points
husarion_ugv_description/urdf/lynx/base.urdf.xacro (3)
47-59: Verify impact of joint position adjustments on robot configuration.The changes to joint positions affect the overall robot height:
- base_to_cover_joint: +1.3mm in Z
- cover_to_mount_joint: -9.8mm in Z
Net change: -8.5mm in total heightPlease confirm:
- These adjustments align with the physical robot's dimensions
- The wheel mount points are correctly positioned relative to these joints
- The changes don't affect any sensors or accessories mounted on the robot
Consider extracting these dimensions as xacro properties at the top of the file for easier maintenance and configuration.
1-64: Verify impact of removed components.The removal of
user_compartmentandbatterycomponents from the URDF represents a significant change to the robot model.Please confirm:
- The battery is now integrated differently in the physical robot
- User compartment functionality is handled through alternative means
- Documentation has been updated to reflect these changes
- Dependent packages have been updated to handle these removals
Consider adding a comment in the URDF explaining the architectural decision to remove these components and how their functionality is now handled.
15-19: Verify the collision mesh properties.The change from a primitive box to a mesh-based collision model is good for accuracy. However, ensure that:
- The collision mesh is a simplified version of the visual mesh
- The mesh is convex or decomposed into convex shapes for efficient collision checking
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the collision mesh exists and examine its properties if [ -f "husarion_ugv_description/meshes/lynx/base_collision.dae" ]; then echo "Collision mesh found" # Print basic info about the mesh file file "husarion_ugv_description/meshes/lynx/base_collision.dae" else echo "Error: Collision mesh not found" exit 1 fiLength of output: 277
🏁 Script executed:
#!/bin/bash # List file details for the collision mesh and preview its content ls -l husarion_ugv_description/meshes/lynx/base_collision.dae echo "-------- First 20 lines of the collision mesh --------" head -n 20 husarion_ugv_description/meshes/lynx/base_collision.daeLength of output: 1131
Collision mesh verification complete.
- The collision mesh file (
husarion_ugv_description/meshes/lynx/base_collision.dae) exists and is in a valid COLLADA format, generated with Blender.- Its file size (6.8 KB) suggests that it is likely a simplified version suitable for collision checking.
- While the mesh structure appears correct, please manually verify that it meets the convexity (or convex decomposition) requirements for efficient collision detection if necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
husarion_ugv_description/urdf/panther/base.urdf.xacro (1)
24-30: Document the reasoning for the inertial coordinates.The specific values chosen for the inertial origin coordinates (-0.008, 0.01, 0.029) appear to be precise measurements. Please add a comment explaining:
- How these values were determined (e.g., CAD model, physical measurements)
- Why this position better represents the robot's center of mass
Add a comment above the inertial section explaining the origin coordinates:
<inertial> + <!-- Center of mass position determined from [source/method] --> <origin xyz="-0.008 0.01 0.029" rpy="0.0 0.0 0.0" />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
husarion_ugv_description/urdf/lynx/base.urdf.xacro(2 hunks)husarion_ugv_description/urdf/panther/base.urdf.xacro(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (5)
husarion_ugv_description/urdf/panther/base.urdf.xacro (1)
24-30: Verify the updated inertial properties in simulation.The center of mass has been shifted from the origin to a new position (-8mm X, +10mm Y, +29mm Z). While this change likely complements the wheel mount point modifications mentioned in the PR, it's crucial to validate its impact on the robot's balance and motion characteristics.
Please ensure that:
- The robot maintains stability in various operating conditions
- The control system performs as expected with these new inertial properties
- The simulation results match the real robot's behavior
husarion_ugv_description/urdf/lynx/base.urdf.xacro (4)
22-28: Verify inertial properties match physical or CAD measurements.The changes to mass and inertia values appear significant:
- Mass increased by ~2% (30.0 → 30.656 kg)
- Center of mass shifted to x=-0.052, y=0.001, z=0.038
- Inertia matrix values updated
These changes will affect the robot's dynamic behavior and control performance.
Could you provide CAD screenshots or physical measurements to validate these values?
47-51: Verify impact of joint position adjustments on robot height.The changes to joint positions affect the overall robot structure:
- Cover joint moved up by 1.3mm
- Mount joint moved down by 9.8mm
- Net decrease in height by 8.5mm
Please confirm these adjustments align with the physical robot dimensions.
Also applies to: 55-59
1-64: Document the removal of user_compartment and battery components.The AI summary indicates that user_compartment and battery components were removed from the URDF. Please:
- Confirm if these removals are intentional
- Update documentation to reflect these changes
- Verify that the power system is properly represented elsewhere in the model
15-20: Verify the collision mesh dimensions and simplicity.The change from a box to a mesh-based collision model is good for accuracy. However, ensure that:
- The collision mesh exists at the specified path
- The collision mesh is simplified compared to the visual mesh for better performance
- The z=0.0 origin aligns with the actual base of the robot
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the collision mesh file exists fd -e dae "base_collision.dae" # List all meshes to compare visual vs collision complexity fd -e dae . --exec echo "File: {}" ";" meshlabserver -i {} -om vc vn fc fn 2>/dev/nullLength of output: 165
🏁 Script executed:
#!/bin/bash # List all .dae files in the repository to identify visual vs collision meshes echo "Listing all .dae files:" fd -e dae # Count the number of triangles in the collision mesh to assess its complexity echo "Counting <triangles occurrences in husarion_ugv_description/meshes/lynx/base_collision.dae:" grep -o "<triangles" husarion_ugv_description/meshes/lynx/base_collision.dae | wc -lLength of output: 1301
Collision Mesh Verification Passed
- File Verification: The collision mesh file
husarion_ugv_description/meshes/lynx/base_collision.daeexists.- Mesh Complexity: The file reports only 1 occurrence of
<triangles, which indicates that the collision geometry is highly simplified.- Origin Alignment: The
<origin xyz="0.0 0.0 0.0" ... />correctly places the collision geometry at the base as intended.These results confirm that the collision mesh is simplified compared to a detailed visual mesh and the configuration fits the intended design.
| <xacro:property name="wheelbase" value="0.44" /> | ||
| <xacro:property name="wheel_mount_point_y" value="0.2575" /> |
There was a problem hiding this comment.
Probably better mor technical names for these properties:
https://www.researchgate.net/publication/325923660/figure/fig3/AS:640246341443584@1529658031027/Positioning-of-the-track-width-wheel-base-and-centre-of-gravity.png
There was a problem hiding this comment.
Wheelbase is a technical name and is often written as single word.
The wheel_mount_point_y is not related to track of the wheels. It is actually a point where wheel together with a hub connects to the robot. This point is almost at the side of the robots body
| @@ -2,36 +2,49 @@ | |||
| <robot xmlns:xacro="http://wiki.ros.org/xacro"> | |||
|
|
|||
| <!-- wheel defining macro --> | |||
| <xacro:macro name="wheel" params="wheel_config wheel_separation_x prefix"> | |||
| <xacro:macro name="wheel" | |||
| params="wheel_config prefix wheel_mount_point_x wheel_mount_point_y"> | |||
There was a problem hiding this comment.
Names like: wheel_base wheel_track is probably better and mor specific names for these properties:
https://www.researchgate.net/publication/325923660/figure/fig3/AS:640246341443584[@1529658031027](https://github.com/1529658031027)/Positioning-of-the-track-width-wheel-base-and-centre-of-gravity.png
/Positioning-of-the-track-width-wheel-base-and-centre-of-gravity.png](https://www.researchgate.net/publication/325923660/figure/fig3/AS:640246341443584%5B@1529658031027%5D(https://github.com/1529658031027)/Positioning-of-the-track-width-wheel-base-and-centre-of-gravity.png){kind=link}
There was a problem hiding this comment.
As described above this is the mount point for the wheel not exactly position of the wheel itself
| <xacro:property name="wheelbase" value="0.35" /> | ||
| <xacro:property name="wheel_mount_point_y" value="0.1675" /> |
There was a problem hiding this comment.
Names like: wheel_base wheel_track is probably better and mor specific names for these properties:
https://www.researchgate.net/publication/325923660/figure/fig3/AS:640246341443584[@1529658031027](https://github.com/1529658031027)/Positioning-of-the-track-width-wheel-base-and-centre-of-gravity.png
Description
Requirements
Tests 🧪
Summary by CodeRabbit
New Features
Refactor